-
Notifications
You must be signed in to change notification settings - Fork 755
Run fake dojo tests on every PR #252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
250fcf5
to
60f85e9
Compare
621731c
to
278f0c6
Compare
run: | | ||
node ../scripts/run-dojo-everything.js & | ||
npx wait-port 9999 | ||
sleep 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this leave us open to flakey tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could but so far even in CI everything is running in <5 seconds from dojo coming up, so I figured 10 seconds is a big enough buffer.
Ideally we can add a health check to all of the agents and make sure they're all running before we move on, but I didn't want to block on that.
|
||
const nextConfig: NextConfig = { | ||
/* config options here */ | ||
output: "standalone", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had added this before for the old contract testing, but since we're not deploying this, I can take it back out. It makes the startup process more consistent to not have this when linking in copilotkit local-dev versions.
|
||
if __name__ == "__main__": | ||
agui_app.serve(app="agent:app", host="0.0.0.0", port=9001, reload=True) | ||
port = int(os.getenv("PORT", "9001")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were these docs out of date and you're just updating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old version forced running on port 9001. Now it looks for a PORT environment variable, and if that's not present, defaults to 9001. Just makes it easier to run agents locally if you can configure their port (so we can avoid collisions)
b273899
to
072d359
Compare
Small comment, removed the pnpm lock file from the dojo because it's unused, it just uses the root one. |
@maxkorp fwiw, I found the timing on the tests to be a bit finicky, at least when running against the ADK implementation. Here's what I ended up with, which works somewhat reliably : |
* Add scripts for running dojo for e2e * make examples have configurable ports * Add new temp e2e suite * Add github workflow to run on PRs * misc file cleanup * Fix runtime error if no readme * remove unneeded mdx providers to allow build * bump pnpm to 10.13.1 to align with copilotkit repo
This causes a fake test suite of e2e tests (to be replaced with the real tests once those are in better shape) on every PR.
Also does some small maintenance to allow us to use this for contract testing, like removing some unneeded use of MDXProvider that was getting into some sort of hoisting issue and causing obscure type errors. We're passing in the components anyways so we don't need the provider.
Also fixes a small bug with loading the dojo when readme's are missing, and bumps pnpm version to 10.something.somethingelse.